Skip to content

Conversation

@adolgachev
Copy link
Contributor

@adolgachev adolgachev commented Jan 29, 2026

The angular-aria keyboard manager ignores the event.repeat parameter so holding down a key will cause it to loop even when that behavior is not desired. In general, repeated events should only be handled when navigating with arrow keys, and especially not when toggling (as then it just goes from off to on to off and so on).

Another issue is that the browser can interpret a keyboard space or enter as a click in certain cases. This is true with the toolbar so added check for a proper mouse event to handle the click (else already handled with keyboard) as well as updated spec to pass a proper event.

Note: we are missing tests for this but they probably need to be added at a higher level so that we can catch for not just the repeat issue but also issues like the click above. Else can just add explicit repeat but keyDown events but that isn't as useful.

Fixes b/479281429.

@adolgachev adolgachev changed the title fix(aria/many): don't repeat keys fix(multiple): don't repeat keys Feb 3, 2026
@adolgachev adolgachev changed the title fix(multiple): don't repeat keys fix(aria/many): change aria keyboard manager to only handle repeated events in correct places Feb 4, 2026
@adolgachev adolgachev changed the title fix(aria/many): change aria keyboard manager to only handle repeated events in correct places fix(multiple): change aria keyboard manager to only handle repeated events in correct places Feb 4, 2026
@adolgachev adolgachev marked this pull request as ready for review February 4, 2026 22:54
@adolgachev adolgachev requested a review from ok7sai February 4, 2026 22:54
@pullapprove pullapprove bot requested a review from andrewseguin February 4, 2026 22:54
@adolgachev adolgachev requested review from tjshiu and removed request for andrewseguin February 4, 2026 22:55
@adolgachev adolgachev added Accessibility This issue is related to accessibility (a11y) target: patch This PR is targeted for the next patch release action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Feb 4, 2026
* This library has not yet had a need for stopPropagationImmediate.
*/
export interface EventHandlerOptions {
handleRepeat?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might prefer preventRepeat instead. It makes it clearer what it is doing. I wasn't sure at first what handleRepeat was doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had ignoreRepeat but then I changed the default to ignore the repeat, and it is generally better to have defaults be false.

But we actually do default the others as true already, so I'll go back to that.

@adolgachev adolgachev removed the request for review from ok7sai February 10, 2026 22:20
@adolgachev adolgachev added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Feb 10, 2026
@adolgachev adolgachev merged commit 529c1bf into angular:main Feb 10, 2026
25 of 27 checks passed
@adolgachev
Copy link
Contributor Author

This PR was merged into the repository. The changes were merged into the following branches:

adolgachev added a commit that referenced this pull request Feb 10, 2026
…vents in correct places (#32728)

(cherry picked from commit 529c1bf)
@adolgachev adolgachev deleted the aria-kb-repeat branch February 10, 2026 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants